Skip to content

feat: add structured logging with request correlation#115

Merged
rubenhensen merged 1 commit into
mainfrom
feat/structured-logging
Jul 1, 2026
Merged

feat: add structured logging with request correlation#115
rubenhensen merged 1 commit into
mainfrom
feat/structured-logging

Conversation

@rubenhensen

Copy link
Copy Markdown
Contributor

Closes #96.

Replaces ad-hoc console.* calls with a structured logger and per-request correlation.

Changes

  • src/lib/server/logger.ts — a pino logger writing structured JSON to stdout, level from LOG_LEVEL (default info). No transport, so it stays robust under the bundled adapter-node server.
  • src/hooks.server.ts
    • Derives a request id per request (reuses a sanitised inbound X-Request-Id, else mints one), attaches a child logger to event.locals.log, and echoes X-Request-Id on the response.
    • Logs each request (method, path, status, durationMs) — skipping the /health and /readyz probes to avoid noise.
    • Adds a handleError hook that logs unhandled server errors with the request id + stack.
  • app.d.tsLocals now carries requestId and log.
  • Migrated the existing console.* sites: services/dns-verification.ts (→ base logger) and api/csp-report/+server.ts (→ locals.log).

Notes

  • pino is a runtime dependency (imported by server code); the production image already ships node_modules, so it resolves at runtime.
  • The existing dns-verification unit test asserted on console.error; it's been retargeted to the logger mock (same intent — errors are logged with orgId / domain / code). The message field is now err.
  • No pretty-print transport is wired in (transports spawn worker threads that are awkward under the bundled server). Structured JSON is the intended prod format; pipe through pino-pretty locally if desired.

@dobby-coder

dobby-coder Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

On it! I'm reviewing your PR adding structured logging with pino — I'll check it for correctness and rule-compliance and come back with a consolidated review.

@dobby-coder dobby-coder Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the structured-logging change against Dobby's rule set and the diff itself.

Rule compliance: clean. Conventional-commit title (feat:), opened ready-for-review (not draft), pino is genuinely imported by server code (not a dead bump), vi.hoisted() mock added correctly, and the retargeted dns-verification test preserves the original assertions' intent. No rule violations.

Verification (from review): built with adapter-node (pino externalized, resolves at runtime), 83 vitest tests pass, svelte-check clean, and a run of the built server confirmed structured JSON logging, X-Request-Id echo + sanitisation (abc/../123 evil -> abc123evil), /health excluded from logs, and locals.log.warn firing on /api/csp-report.

One minor nit inline — non-blocking.

Comment thread src/hooks.server.ts

// Skip the noisy liveness/readiness probes.
const path = event.url.pathname;
if (path !== '/health' && path !== '/readyz') {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the request-log skip excludes both /health and /readyz, but there's no /readyz route in the repo (only /health). Harmless dead condition — either add the readiness probe or drop the && path !== '/readyz' clause.

@dobby-coder

dobby-coder Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Reviewed! Everything looks clean — conventional commit title, pino is genuinely used, and the vi.hoisted mock is correct. I left one inline nit on src/hooks.server.ts:51: the /readyz skip clause references a route that doesn't seem to exist in the app, so worth a quick check there. Non-blocking though, happy to approve once that's confirmed. 🐾

@rubenhensen

Copy link
Copy Markdown
Contributor Author

Thanks @dobby-coder — on the /readyz skip: it's intentional and forward-looking. The /readyz route is added in #113 (still open); once that merges, this skip keeps the readiness poll out of the request log alongside /health. On this branch in isolation it's a harmless no-op (the condition simply never matches) rather than a bug, so I've left it in to avoid re-adding it later and creating readiness-probe log noise. Happy to drop it if you'd prefer #113 to own the skip instead.

@rubenhensen rubenhensen merged commit 4290278 into main Jul 1, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Observability: adopt structured logging with request correlation (replace ad-hoc console.*)

1 participant